fix: maxInnerIterationsPerGrowthAttempt was silently ignored - #1776
Closed
apullin wants to merge 1 commit into
Closed
fix: maxInnerIterationsPerGrowthAttempt was silently ignored#1776apullin wants to merge 1 commit into
apullin wants to merge 1 commit into
Conversation
GrowShrinkHighDensityIntraNodeSolver bounds a single growth attempt by assigning `activeSubSolver.MAX_ITERATIONS`. PortfolioSingleIntraNodeSolver then recomputes MAX_ITERATIONS in refreshDynamicIterationLimit() from the remaining candidate budgets - on its first step, and again on every portfolio expansion - so the cap was discarded immediately. Observed on the added fixture: a cap of 25 becomes 45128. The parameter has therefore never had any effect on this path, and a growth attempt always ran to the portfolio's own dynamic budget before the node was grown and retried. Fix: an explicit `externalMaxIterations` ceiling on the portfolio that the dynamic limit respects, set by GrowShrink alongside the existing assignment. Tests: tests/features/never-fail-growth-high-density/caps-inner-iterations.test.ts covers both the wiring (a busy 4-connection node whose inner solver is still running after one step) and the portfolio ceiling itself. Both fail before this change and pass after. Suite: 419 pass / 55 skip / 2 fail; the two failures (bugreport36-d4c6c2 and "dip16 crossing traces 1206x4") reproduce identically on an unmodified checkout of this commit's parent, so they are pre-existing and unrelated.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Benchmark This PRRun benchmarks by commenting on this PR: Comment Everything after Use Any PR whose title contains |
apullin
pushed a commit
to apullin/tscircuit-autorouter
that referenced
this pull request
Jul 28, 2026
…utorouter tscircuit#1776); issues still drafted
Contributor
|
/benchmark-all |
Contributor
Default Benchmark ResultsPrevious main runDataset: dataset01
Previous main run details
Dataset: dataset01
PR run details
Workflow: View run |
Contributor
srj18 ResultsPrevious main runDataset: srj18
Previous main run details
Dataset: srj18
PR run details
Workflow: View run |
Contributor
srj19 ResultsPrevious main runDataset: srj19
Previous main run details
Dataset: srj19
PR run details
Workflow: View run |
Contributor
srj20 ResultsPrevious main runDataset: srj20
Previous main run details
Dataset: srj20
PR run details
Workflow: View run |
Contributor
srj21 ResultsPrevious main runDataset: srj21
Previous main run details
Dataset: srj21
PR run details
Workflow: View run |
Contributor
srj23 ResultsPrevious main runDataset: srj23
Previous main run details
Dataset: srj23
PR run details
Workflow: View run |
Contributor
|
@apullin can you give us some context on your tscircuit usage? Will help us understand if the ai generated PRs are based in real usage (which validates them more) |
Author
|
@seveibar |
Contributor
|
CC @ShiboSoftwareDev for review @apullin we will merge more quickly w/ context that helps build trust in the contributor/maintainer. Are you currently building a board with tscircuit tsx? Or is the autorouter used standalone? In general, what is the complexity of the board you are routing in terms of net count, or dominant chip (e.g. RK3566, RP2040 etc.) If you're just here for optimization, that's also ok- but we will need to have a maintainer help to make sure your changes get upstream through the pipeline (often optimizers have a more narrow outlook on the objectives, and don't necessarily see the upstream visual snapshots) We can also help share insights in either case, e.g. we ran experiments on high density parallelism as well as high density lossy caching |
seveibar
reviewed
Jul 28, 2026
seveibar
left a comment
seveibar
left a comment
Contributor
There was a problem hiding this comment.
I don't think i can review because this is @ShiboSoftwareDev 's system, it looks OK to me, there may be a small negative performance impact based on the benchmark results
Contributor
|
the perf impact is likely to be negligible for this one in particular- but this could just be a set up for a future optimization |
Author
|
@seveibar No boards started yet. Will do some S100 boards eventually. Only looking at speedups on existing benchmarks. |
Contributor
|
cool- it can be a useful way to report on optimization results if you PR a bugreport for a board you're trying to route- https://docs.tscircuit.com/contributing/report-autorouter-bugs also note that there's a lot of discussion on discord (https://tscircuit.com/join) in the #autorouting channel about various approaches and optimizations. We're currently more focused on predefined trace influence (for staged autorouting), power traces, ddr3 routing etc. than optimization. That said, a good optimization PR is appreciated especially if it doesn't come at the cost of code clarity glad to have you here, normally reviews go pretty quick but i will leave this particular subsystem to @ShiboSoftwareDev |
Contributor
|
/benchmark --dataset 18 --sample-timeout 2000s |
Contributor
|
/benchmark --dataset 24 --sample-timeout 2000s |
Contributor
18 ResultsPrevious main runDataset: srj18
Previous main run details
Dataset: srj18
PR run details
Workflow: View run |
Contributor
24 ResultsPrevious main runNo previous main run is available for Dataset: srj24
PR run details
Workflow: View run |
|
This PR has been automatically marked as stale because it has had no recent activity. It will be closed if no further activity occurs. |
Author
|
alright, leave the bug in I guess |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GrowShrinkHighDensityIntraNodeSolverbounds a single growth attempt byassigning
activeSubSolver.MAX_ITERATIONS.PortfolioSingleIntraNodeSolverthen recomputes
MAX_ITERATIONSinrefreshDynamicIterationLimit()from theremaining candidate budgets — on its first step, and again on every portfolio
expansion — so the cap was discarded immediately. Observed on the added
fixture: a cap of 25 becomes 45,128.
The
maxInnerIterationsPerGrowthAttemptparameter has therefore never had anyeffect on this path, and a growth attempt always ran to the portfolio's own
dynamic budget before the node was grown and retried.
Fix: an explicit
externalMaxIterationsceiling on the portfolio that thedynamic limit respects, set by GrowShrink alongside the existing assignment.
No production caller currently sets the parameter (neither pipeline supplies
it), so default behavior is byte-identical; the fix only makes the documented
parameter actually work for callers who pass it.
Tests:
tests/features/never-fail-growth-high-density/caps-inner-iterations.test.tscovers both the wiring (a busy 4-connection node whose inner solver is still
running after one step) and the portfolio ceiling itself. Both fail before this
change and pass after.
Suite: 419 pass / 55 skip / 2 fail; the two failures (
bugreport36-d4c6c2anddip16 crossing traces 1206x4) reproduce identically on an unmodified checkoutof this commit's parent (v0.0.717), so they are pre-existing and unrelated.